Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(expr): support IGNORE NULLS for first_value/last_value #19847

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat(expr): support IGNORE NULLS for first_value/last_value #19847

wants to merge 10 commits into from

Conversation

stdrc
Copy link
Member

@stdrc stdrc commented Dec 18, 2024 •
edited
Loading

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR implements IGNORE NULLS support for first_value and last_value window functions, similar to duckdb.

Syntax:

select
    *,
    first_value(val) over (partition by 1::int order by ts) as first,
    first_value(val ignore nulls) over (partition by 1::int order by ts) as first_non_null,
    first_value(val ignore nulls) over (partition by 1::int order by ts rows between 1 preceding and 1 following) as first_non_null_2,
    last_value(val) over (partition by 1::int order by ts) as last,
    last_value(val ignore nulls) over (partition by 1::int order by ts) as last_non_null,
    last_value(val ignore nulls) over (partition by 1::int order by ts rows between 1 preceding and 1 following) as last_non_null_2
from t;

Fixes #17601.

lag and lead still don't support this syntax due to some historical reason. Will implement for them later.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

  • Support IGNORE NULLS for first_value and last_value window functions. Syntax:

    first_value(val IGNORE NULLS) OVER (...)

@stdrc Graphite App
Copy link
Member Author

stdrc commented Dec 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@stdrc stdrc changed the title add ignore_nulls field to frontend/expr/WindowFunction feat(expr): support IGNORE NULLS for first_value/last_value Dec 18, 2024
@stdrc stdrc marked this pull request as ready for review December 18, 2024 08:08
@stdrc stdrc requested review from yuhao-su, xxchan and st1page December 18, 2024 08:19
@stdrc stdrc added the user-facing-changes Contains changes that are visible to users label Dec 18, 2024
@github-actions GitHub Actions
Copy link
Contributor

Hi, there.

📝 Telemetry Reminder:
If you're implementing this feature, please consider adding telemetry metrics to track its usage. This helps us understand how the feature is being used and improve it further.
You can find the function report_event of telemetry reporting in the following files. Feel free to ask questions if you need any guidance!

  • src/frontend/src/telemetry.rs
  • src/meta/src/telemetry.rs
  • src/stream/src/telemetry.rs
  • src/storage/compactor/src/telemetry.rs
    Or calling report_event_common (src/common/telemetry_event/src/lib.rs) as if finding it hard to implement.
    ✨ Thank you for your contribution to RisingWave! ✨

This is an automated comment created by the peaceiris/actions-label-commenter. Responding to the bot or mentioning it won't have any effect.

stdrc added 10 commits December 18, 2024 17:32
@stdrc
add ignore_nulls field to frontend/expr/WindowFunction
305d9cf 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
add ignore_nulls to WindowFunction proto
b4860b4 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
fix debug print
aadecee 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
ban lag/lead with IGNORE NULLS
a05285e 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
support IGNORE NULLS for first_value & last_value
b68e965 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
add e2e
293714b 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
remove todo
e159d86 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
add planner tests
5a1d08b 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
fix ut and check
3c3904b 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc
fix test
2d21a29 
Signed-off-by: Richard Chien <stdrc@outlook.com>
@stdrc stdrc force-pushed the rc/window-function-ignore-nulls branch from 4e597f5 to 2d21a29 Compare December 18, 2024 09:33
@graphite-app graphite-app bot requested a review from a team December 18, 2024 10:13
@stdrc stdrc requested a review from lmatz December 19, 2024 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmatz lmatz lmatz approved these changes

@yuhao-su yuhao-su Awaiting requested review from yuhao-su

@xxchan xxchan Awaiting requested review from xxchan

@st1page st1page Awaiting requested review from st1page

Assignees
No one assigned
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ignore nulls in over window function
2 participants
@stdrc @lmatz